[Tablegen] Bugfix and refactor VarLenCodeEmitter HwModes. #68795
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
VarLenCodeEmitterGen produced code that did not compile if using
alternative encoding in different HwModes. It's not possbile to assign
unsigned **Index = Index_<mode>[][2] = { ... };
As a fix, Index and InstBits where removed in favor of mode specific
getInstBits_<mode> functions since this is the only place the arrays are
accessed.
Handling of HwModes is now concentrated to the VarLenCodeEmitterGen::run
method reducing the overall amount of code and enabling other types of
alternative encodings not related to HwModes.
Added a test for VarLenCodeEmitterGen HwModes.
Make sure that HwModes are supported in the same way they are supported
for the standard CodeEmitter. It should be possible to define
instructions with universal encoding across modes, distinct encodings
for each mode or only define encodings for some modes.
Fixed indentation in generated code.
|
Hi @mshockwave, I was just wondering if you have seen this patch? And if so, if you have any comments. |
mshockwave
reviewed
Oct 18, 2023
There was a problem hiding this comment.
Thanks for fixing this issue! Indeed we didn't thoroughly test the case where each HW mode has different encoding.
Could you help me to confirm that this doesn't break anything for the 68k backend, which is the only user of VarLenCodeEmitter upstream? It's not built by default and you need to use -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=M68k to enable it when running cmake.
Thanks!
| << " };\n"; | ||
| for (const auto &Mode : Modes) { | ||
| // Emit helper function to retrieve base values. | ||
| OS << " auto getInstBits" << Mode.second |
There was a problem hiding this comment.
Could you also show in the test case that getInstBits directly accesss Index<mode> rather than Index?
There was a problem hiding this comment.
I have now extended the test case with checks for accesses to Index<mode> and getInstBits<mode>.
When testing I rebased the patch onto 9ea2fd245739fe7d8f49014f90d2311387bf7682 which at the time was the latest commit on main. Then I successfully ran the check-llvm lit tests while greping out 137 tests with M68k in the name.
$ llvm-lit test/ | grep M68k
PASS: LLVM :: CodeGen/M68k/reserved-regs.ll (2870 of 51650)
PASS: LLVM :: CodeGen/M68k/Atomics/rmw.ll (4503 of 51650)
PASS: LLVM :: CodeGen/M68k/Atomics/load-store.ll (4956 of 51650)
PASS: LLVM :: CodeGen/M68k/Atomics/cmpxchg.ll (7711 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/bitwise.ll (19953 of 51650)
PASS: LLVM :: CodeGen/M68k/link-unlnk.ll (20623 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/divide-by-constant.ll (21283 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/sub.ll (22976 of 51650)
PASS: LLVM :: CodeGen/M68k/m6888x-features.ll (24809 of 51650)
PASS: LLVM :: CodeGen/M68k/Control/cmp.ll (25253 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/smul-with-overflow.ll (25418 of 51650)
PASS: LLVM :: CodeGen/M68k/CodeModel/small-static.ll (27542 of 51650)
PASS: LLVM :: CodeGen/M68k/CodeModel/small-pie-global-access.ll (27652 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/umul-with-overflow.ll (27752 of 51650)
PASS: LLVM :: CodeGen/M68k/CodeModel/small-pic.ll (27856 of 51650)
PASS: LLVM :: CodeGen/M68k/CConv/c-call.ll (28081 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/add-with-overflow.ll (28609 of 51650)
PASS: LLVM :: CodeGen/M68k/CConv/c-args.ll (28610 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/imul.ll (28742 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/sdiv-exact.ll (28844 of 51650)
PASS: LLVM :: CodeGen/M68k/CConv/fastcc-call.ll (29041 of 51650)
PASS: LLVM :: CodeGen/M68k/PR57660.ll (29142 of 51650)
PASS: LLVM :: DebugInfo/M68k/calling-convention.ll (29974 of 51650)
PASS: LLVM :: CodeGen/M68k/CConv/rtd-ret.ll (30070 of 51650)
PASS: LLVM :: CodeGen/M68k/CConv/fastcc-args.ll (30085 of 51650)
PASS: LLVM :: CodeGen/M68k/CodeModel/medium-static.ll (30186 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/add.ll (30634 of 51650)
PASS: LLVM :: CodeGen/M68k/CodeModel/medium-pie-global-access.ll (31116 of 51650)
PASS: LLVM :: CodeGen/M68k/CodeModel/medium-pie.ll (31225 of 51650)
PASS: LLVM :: CodeGen/M68k/CConv/c-args-inreg.ll (31519 of 51650)
PASS: LLVM :: CodeGen/M68k/CodeModel/medium-pic.ll (31584 of 51650)
PASS: LLVM :: CodeGen/M68k/GlobalISel/legalize-udiv.mir (31701 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/sub-with-overflow.ll (31735 of 51650)
PASS: LLVM :: CodeGen/M68k/GlobalISel/irtranslator-ret.ll (32114 of 51650)
PASS: LLVM :: CodeGen/M68k/Atomics/fence.ll (32486 of 51650)
PASS: LLVM :: CodeGen/M68k/Alloc/dyn_alloca_aligned.ll (32527 of 51650)
PASS: LLVM :: CodeGen/M68k/ShiftRotate/asr.ll (32658 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/imul-neg.ll (32700 of 51650)
PASS: LLVM :: CodeGen/M68k/GlobalISel/irtranslator-call.ll (32922 of 51650)
PASS: LLVM :: CodeGen/M68k/Control/long-setcc.ll (32971 of 51650)
PASS: LLVM :: CodeGen/M68k/inline-asm.ll (33280 of 51650)
PASS: LLVM :: CodeGen/M68k/CConv/rtd-call.ll (33281 of 51650)
PASS: LLVM :: CodeGen/M68k/Control/setcc.ll (33979 of 51650)
PASS: LLVM :: CodeGen/M68k/TLS/tlsie.ll (34121 of 51650)
PASS: LLVM :: CodeGen/M68k/ShiftRotate/rol.ll (34601 of 51650)
PASS: LLVM :: CodeGen/M68k/gcc_except_table.ll (34890 of 51650)
PASS: LLVM :: CodeGen/M68k/GlobalISel/legalize-and.mir (35088 of 51650)
PASS: LLVM :: CodeGen/M68k/GlobalISel/arithmetic.ll (35158 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/mul_div_32.ll (35280 of 51650)
PASS: LLVM :: CodeGen/M68k/GlobalISel/legalize-mul.mir (35428 of 51650)
PASS: LLVM :: CodeGen/M68k/GlobalISel/legalize-add.mir (35645 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/lshr.ll (35737 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/sext-i1.ll (35811 of 51650)
PASS: LLVM :: CodeGen/M68k/CollapseMOVEM.mir (35994 of 51650)
PASS: LLVM :: CodeGen/M68k/Arith/mul64.ll (36675 of 51650)
PASS: LLVM :: CodeGen/M68k/GlobalISel/irtranslator-pic.ll (37699 of 51650)
PASS: LLVM :: CodeGen/M68k/GlobalISel/legalize-load-store.mir (37924 of 51650)
PASS: LLVM :: CodeGen/M68k/CodeModel/small-pie.ll (37996 of 51650)
PASS: LLVM :: CodeGen/M68k/varargs.ll (38106 of 51650)
PASS: LLVM :: CodeGen/M68k/ShiftRotate/lsl.ll (38562 of 51650)
PASS: LLVM :: CodeGen/M68k/TLS/tlsld.ll (38815 of 51650)
PASS: LLVM :: CodeGen/M68k/TLS/tlsgd.ll (38939 of 51650)
PASS: LLVM :: CodeGen/M68k/ShiftRotate/ror.ll (39006 of 51650)
PASS: LLVM :: CodeGen/M68k/multiple-return.ll (39622 of 51650)
PASS: LLVM :: CodeGen/M68k/TLS/tlsle.ll (39790 of 51650)
PASS: LLVM :: CodeGen/M68k/load-extend.ll (39986 of 51650)
PASS: LLVM :: CodeGen/M68k/GlobalISel/legalize-sub.mir (40010 of 51650)
PASS: LLVM :: MC/M68k/Bits/Classes/MxBTST_MR.s (40091 of 51650)
PASS: LLVM :: MC/M68k/Bits/Classes/MxBTST_MI.s (40424 of 51650)
PASS: LLVM :: CodeGen/M68k/ShiftRotate/lsr.ll (40455 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxDiMu.s (41065 of 51650)
PASS: LLVM :: MC/M68k/Control/call-pc-rel.s (41082 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxCMP_RR.s (41352 of 51650)
PASS: LLVM :: MC/M68k/Bits/Classes/MxBTST_RI.s (41546 of 51650)
PASS: LLVM :: CodeGen/M68k/GlobalISel/reg_bank_test.ll (41615 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxBiArOp_FMR.s (42084 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxCMP_MI.s (42121 of 51650)
PASS: LLVM :: CodeGen/M68k/is-pcrel-register-operand-legal.mir (42213 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxCMP_RI.s (42382 of 51650)
PASS: LLVM :: CodeGen/M68k/pipeline.ll (42440 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxFUnary_FF.s (42522 of 51650)
PASS: LLVM :: MC/M68k/Control/Classes/MxBRA.s (42639 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxCMP_BI.s (42709 of 51650)
PASS: LLVM :: MC/M68k/Control/Classes/MxBcc.s (42722 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxCMP_RM.s (42784 of 51650)
PASS: LLVM :: MC/M68k/Control/Classes/MxCALL.s (42829 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxExt.s (42871 of 51650)
PASS: LLVM :: MC/M68k/Atomics/cas.s (43001 of 51650)
PASS: LLVM :: MC/M68k/Bits/Classes/MxBTST_RR.s (43027 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxNEG.s (43080 of 51650)
PASS: LLVM :: MC/M68k/Control/Classes/MxJMP.s (43157 of 51650)
PASS: LLVM :: MC/M68k/Control/Classes/MxNOP.s (43174 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxFBinary_FF.s (43257 of 51650)
PASS: LLVM :: MC/M68k/Control/Classes/MxScc.s (44055 of 51650)
PASS: LLVM :: MC/M68k/Control/Classes/MxRTS.s (44133 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxBiArOp_RFRM.s (44158 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxBiArOp_RFRR_xEA.s (44230 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxBiArOp_RFRI_xEA.s (44260 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxBiArOp_RFRR_EAd.s (44404 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxBiArOp_FMI.s (44448 of 51650)
PASS: LLVM :: MC/M68k/Relocations/data-abs.s (44467 of 51650)
PASS: LLVM :: MC/M68k/Data/Classes/MxMOVEM_RM.s (44468 of 51650)
PASS: LLVM :: MC/M68k/Control/branch-pc-rel.s (44478 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxBiArOp_RFRRF.s (44497 of 51650)
PASS: LLVM :: MC/M68k/Relocations/data-gotoff.s (44624 of 51650)
PASS: LLVM :: MC/M68k/Data/Classes/MxMove_MM.s (44684 of 51650)
PASS: LLVM :: MC/M68k/Relaxations/bsr.s (44991 of 51650)
PASS: LLVM :: MC/M68k/Relocations/text-plt.s (45081 of 51650)
PASS: LLVM :: MC/M68k/Control/bsr.s (45133 of 51650)
PASS: LLVM :: MC/M68k/Data/Classes/MxFMove_FF.s (45184 of 51650)
PASS: LLVM :: MC/M68k/Relocations/data-gotpcrel.s (45499 of 51650)
PASS: LLVM :: MC/M68k/Data/Classes/MxLEA.s (45608 of 51650)
PASS: LLVM :: MC/M68k/Arith/Classes/MxBiArOp_RFRI.s (45622 of 51650)
PASS: LLVM :: MC/M68k/Control/trap-break.s (45627 of 51650)
PASS: LLVM :: MC/M68k/Data/Classes/MxMOVEM_MR.s (45628 of 51650)
PASS: LLVM :: MC/M68k/Relaxations/branch.s (45639 of 51650)
PASS: LLVM :: MC/M68k/Relocations/data-pc-rel.s (45699 of 51650)
PASS: LLVM :: MC/M68k/Data/Classes/MxMoveCCR.s (45908 of 51650)
PASS: LLVM :: MC/M68k/Data/Classes/MxLink.s (46012 of 51650)
PASS: LLVM :: MC/M68k/Data/Classes/MxMove_MR.s (46061 of 51650)
PASS: LLVM :: MC/M68k/Data/Classes/MxMove_MI.s (46183 of 51650)
PASS: LLVM :: MC/M68k/Data/Classes/MxMove_RM.s (46650 of 51650)
PASS: LLVM :: MC/M68k/Data/Classes/MxMove_RI.s (46704 of 51650)
PASS: LLVM :: MC/Disassembler/M68k/arithmetic.txt (46889 of 51650)
PASS: LLVM :: MC/M68k/Data/Classes/MxMove_RR.s (47274 of 51650)
PASS: LLVM :: MC/M68k/ShiftRotate/Classes/MxSR_DD.s (47325 of 51650)
PASS: LLVM :: MC/M68k/operand.s (47403 of 51650)
PASS: LLVM :: MC/M68k/instructions.s (47460 of 51650)
PASS: LLVM :: MC/M68k/ShiftRotate/Classes/MxSR_DI.s (47607 of 51650)
PASS: LLVM :: MC/Disassembler/M68k/control.txt (48131 of 51650)
PASS: LLVM :: MC/Disassembler/M68k/trap-break.txt (48520 of 51650)
PASS: LLVM :: MC/Disassembler/M68k/data.txt (48537 of 51650)
PASS: LLVM :: MC/Disassembler/M68k/bits.txt (48732 of 51650)
PASS: LLVM :: MC/Disassembler/M68k/fp-data.txt (49285 of 51650)
PASS: LLVM :: MC/Disassembler/M68k/shift-rotate.txt (49334 of 51650)
PASS: LLVM :: MC/Disassembler/M68k/atomics.txt (49531 of 51650)
PASS: LLVM :: MC/M68k/pc-rel.s (49644 of 51650)
There was a problem hiding this comment.
Perfect! Thank you.
Just a FYI: You can use ninja check-llvm-codegen-<target name> to run only the codegen test for a specific target. Similarly, using ninja check-llvm-mc-<target name> for a target's MC tests.
Extended the VarLenEncoderHwModes.td test with checks for correct usage of Index<mode> and getInstBits<mode>. Also aligned indentation in FileCheck checks.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
VarLenCodeEmitterGen produced code that did not compile if using
alternative encoding in different HwModes. It's not possbile to assign
As a fix, Index and InstBits where removed in favor of mode specific
getInstBits_ functions since this is the only place the arrays are
accessed.
Handling of HwModes is now concentrated to the VarLenCodeEmitterGen::run
method reducing the overall amount of code and enabling other types of
alternative encodings not related to HwModes.
Added a test for VarLenCodeEmitterGen HwModes.
Make sure that HwModes are supported in the same way they are supported
for the standard CodeEmitter. It should be possible to define
instructions with universal encoding across modes, distinct encodings
for each mode or only define encodings for some modes.
Fixed indentation in generated code.